Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable the MaxListenersExceededWarning in Node when creating event targets for internal use #3661

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Disable the MaxListenersExceededWarning in Node when creating event targets for internal use #3661

merged 1 commit into from
Dec 4, 2024

Conversation

steveluscher
Copy link
Collaborator

@steveluscher steveluscher commented Dec 2, 2024 •
edited
Loading

Summary

Read all about why, here: https://solana.stackexchange.com/a/17971/75

Fixes #3624.

Test Plan

const {createSolanaRpcSubscriptions} = require('./dist/index.node.cjs');

const abortController = new AbortController();
const solanaRpcSubscriptions = createSolanaRpcSubscriptions('ws://localhost:8900');

const sub1 = solanaRpcSubscriptions.logsNotifications(
        { mentions: ['1'.repeat(32)] },
        { commitment: "processed" }).subscribe({abortSignal: abortController.signal}).then(() => {});
const sub2 = solanaRpcSubscriptions.logsNotifications(
        { mentions: ['1'.repeat(32)] },
        { commitment: "confirmed" }).subscribe({abortSignal: abortController.signal}).then(() => {});
const sub3 = solanaRpcSubscriptions.logsNotifications(
        { mentions: ['1'.repeat(32)] },
        { commitment: "finalized" }).subscribe({abortSignal: abortController.signal}).then(() => {});

No more warning.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 2, 2024

🦋 Changeset detected

Latest commit: e03b47f

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@steveluscher Graphite App
Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

steveluscher
steveluscher commented Dec 2, 2024
View reviewed changes
Copy link
Collaborator Author

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of this? It's pretty heavy handed, but probably correct given how we use event targets internally to implement publish/subscribe/demux.

@bundlemon BundleMon
Copy link

bundlemon bot commented Dec 2, 2024

BundleMon

Files added (2)
Status Path Size Limits
✅ event-target-impl/dist/index.node.mjs
 +233B -
✅ event-target-impl/dist/index.browser.mjs
 +133B -
Files updated (17)
Status Path Size Limits
✅ subscribable/dist/index.node.mjs
 1.8KB (+113B +6.52%) -
✅ transaction-confirmation/dist/index.node.mjs
 2.4KB (+102B +4.33%) -
✅ rpc-subscriptions-spec/dist/index.node.mjs
 2.13KB (+99B +4.75%) -
✅ rpc/dist/index.node.mjs
 1.95KB (+92B +4.82%) -
✅ rpc-subscriptions/dist/index.node.mjs
 3.34KB (+91B +2.73%) -
✅ rpc-subscriptions-channel-websocket/dist/inde
x.node.mjs
 1.33KB (+81B +6.33%) -
✅ @solana/web3.js production bundle
library/dist/index.production.min.js
 33.82KB (+63B +0.18%) -
✅ subscribable/dist/index.native.mjs
 1.75KB (+55B +3.17%) -
✅ rpc-subscriptions/dist/index.native.mjs
 3.31KB (+53B +1.59%) -
✅ rpc-subscriptions-spec/dist/index.native.mjs
 2.08KB (+49B +2.35%) -
✅ subscribable/dist/index.browser.mjs
 1.74KB (+48B +2.77%) -
✅ transaction-confirmation/dist/index.native.mj
s
 2.34KB (+47B +2%) -
✅ rpc-subscriptions/dist/index.browser.mjs
 3.38KB (+45B +1.32%) -
✅ rpc/dist/index.native.mjs
 1.8KB (+45B +2.5%) -
✅ rpc-subscriptions-spec/dist/index.browser.mjs
 2.08KB (+42B +2.01%) -
✅ transaction-confirmation/dist/index.browser.m
js
 2.34KB (+40B +1.7%) -
✅ rpc/dist/index.browser.mjs
 1.8KB (+37B +2.05%) -
Unchanged files (108)
Status Path Size Limits
✅ rpc-graphql/dist/index.browser.mjs
 18.79KB -
✅ rpc-graphql/dist/index.native.mjs
 18.79KB -
✅ rpc-graphql/dist/index.node.mjs
 18.79KB -
✅ errors/dist/index.node.mjs
 14KB -
✅ errors/dist/index.browser.mjs
 13.99KB -
✅ errors/dist/index.native.mjs
 13.98KB -
✅ transaction-messages/dist/index.browser.mjs
 7.05KB -
✅ transaction-messages/dist/index.native.mjs
 7.05KB -
✅ transaction-messages/dist/index.node.mjs
 7.05KB -
✅ codecs-data-structures/dist/index.native.mjs
 4.77KB -
✅ codecs-data-structures/dist/index.browser.mjs
 4.77KB -
✅ codecs-data-structures/dist/index.node.mjs
 4.77KB -
✅ webcrypto-ed25519-polyfill/dist/index.node.mj
s
 3.49KB -
✅ webcrypto-ed25519-polyfill/dist/index.browser
.mjs
 3.47KB -
✅ webcrypto-ed25519-polyfill/dist/index.native.
mjs
 3.45KB -
✅ codecs-core/dist/index.browser.mjs
 3.3KB -
✅ codecs-core/dist/index.native.mjs
 3.3KB -
✅ codecs-core/dist/index.node.mjs
 3.3KB -
✅ rpc-transformers/dist/index.browser.mjs
 2.92KB -
✅ rpc-transformers/dist/index.native.mjs
 2.92KB -
✅ rpc-transformers/dist/index.node.mjs
 2.92KB -
✅ addresses/dist/index.browser.mjs
 2.8KB -
✅ addresses/dist/index.native.mjs
 2.8KB -
✅ addresses/dist/index.node.mjs
 2.8KB -
✅ library/dist/index.browser.mjs
 2.67KB -
✅ library/dist/index.native.mjs
 2.67KB -
✅ library/dist/index.node.mjs
 2.67KB -
✅ codecs-strings/dist/index.browser.mjs
 2.53KB -
✅ signers/dist/index.browser.mjs
 2.53KB -
✅ signers/dist/index.native.mjs
 2.53KB -
✅ signers/dist/index.node.mjs
 2.53KB -
✅ codecs-strings/dist/index.node.mjs
 2.48KB -
✅ codecs-strings/dist/index.native.mjs
 2.46KB -
✅ sysvars/dist/index.browser.mjs
 2.32KB -
✅ sysvars/dist/index.native.mjs
 2.31KB -
✅ sysvars/dist/index.node.mjs
 2.31KB -
✅ codecs-numbers/dist/index.native.mjs
 2.01KB -
✅ codecs-numbers/dist/index.browser.mjs
 2.01KB -
✅ codecs-numbers/dist/index.node.mjs
 2.01KB -
✅ transactions/dist/index.browser.mjs
 1.99KB -
✅ transactions/dist/index.native.mjs
 1.99KB -
✅ transactions/dist/index.node.mjs
 1.99KB -
✅ react/dist/index.native.mjs
 1.98KB -
✅ react/dist/index.browser.mjs
 1.98KB -
✅ react/dist/index.node.mjs
 1.98KB -
✅ rpc-transport-http/dist/index.browser.mjs
 1.91KB -
✅ rpc-transport-http/dist/index.native.mjs
 1.91KB -
✅ keys/dist/index.browser.mjs
 1.86KB -
✅ keys/dist/index.native.mjs
 1.86KB -
✅ keys/dist/index.node.mjs
 1.85KB -
✅ rpc-transport-http/dist/index.node.mjs
 1.75KB -
✅ rpc-types/dist/index.browser.mjs
 1.6KB -
✅ rpc-types/dist/index.native.mjs
 1.6KB -
✅ rpc-types/dist/index.node.mjs
 1.6KB -
✅ rpc-subscriptions-channel-websocket/dist/inde
x.native.mjs
 1.27KB -
✅ rpc-subscriptions-channel-websocket/dist/inde
x.browser.mjs
 1.26KB -
✅ options/dist/index.browser.mjs
 1.18KB -
✅ options/dist/index.native.mjs
 1.18KB -
✅ options/dist/index.node.mjs
 1.17KB -
✅ accounts/dist/index.browser.mjs
 1.13KB -
✅ accounts/dist/index.native.mjs
 1.12KB -
✅ accounts/dist/index.node.mjs
 1.12KB -
✅ rpc-spec-types/dist/index.browser.mjs
 964B -
✅ rpc-api/dist/index.browser.mjs
 963B -
✅ rpc-api/dist/index.native.mjs
 962B -
✅ rpc-spec-types/dist/index.native.mjs
 962B -
✅ rpc-spec-types/dist/index.node.mjs
 961B -
✅ rpc-api/dist/index.node.mjs
 960B -
✅ rpc-subscriptions-api/dist/index.native.mjs
 870B -
✅ rpc-subscriptions-api/dist/index.node.mjs
 869B -
✅ rpc-subscriptions-api/dist/index.browser.mjs
 868B -
✅ rpc-spec/dist/index.browser.mjs
 829B -
✅ rpc-spec/dist/index.native.mjs
 829B -
✅ rpc-spec/dist/index.node.mjs
 828B -
✅ promises/dist/index.browser.mjs
 799B -
✅ promises/dist/index.native.mjs
 798B -
✅ promises/dist/index.node.mjs
 797B -
✅ assertions/dist/index.browser.mjs
 790B -
✅ instructions/dist/index.browser.mjs
 771B -
✅ instructions/dist/index.native.mjs
 770B -
✅ instructions/dist/index.node.mjs
 768B -
✅ assertions/dist/index.native.mjs
 724B -
✅ fast-stable-stringify/dist/index.browser.mjs
 724B -
✅ assertions/dist/index.node.mjs
 723B -
✅ fast-stable-stringify/dist/index.native.mjs
 723B -
✅ fast-stable-stringify/dist/index.node.mjs
 722B -
✅ compat/dist/index.browser.mjs
 712B -
✅ compat/dist/index.native.mjs
 711B -
✅ compat/dist/index.node.mjs
 710B -
✅ programs/dist/index.browser.mjs
 329B -
✅ programs/dist/index.native.mjs
 327B -
✅ programs/dist/index.node.mjs
 325B -
✅ functional/dist/index.browser.mjs
 154B -
✅ functional/dist/index.native.mjs
 152B -
✅ text-encoding-impl/dist/index.native.mjs
 152B -
✅ functional/dist/index.node.mjs
 151B -
✅ codecs/dist/index.browser.mjs
 137B -
✅ codecs/dist/index.native.mjs
 136B -
✅ codecs/dist/index.node.mjs
 134B -
✅ ws-impl/dist/index.node.mjs
 131B -
✅ text-encoding-impl/dist/index.browser.mjs
 122B -
✅ text-encoding-impl/dist/index.node.mjs
 119B -
✅ crypto-impl/dist/index.node.mjs
 114B -
✅ ws-impl/dist/index.browser.mjs
 113B -
✅ crypto-impl/dist/index.browser.mjs
 109B -
✅ rpc-parsed-types/dist/index.browser.mjs
 66B -
✅ rpc-parsed-types/dist/index.native.mjs
 65B -
✅ rpc-parsed-types/dist/index.node.mjs
 63B -

Total files change +1.44KB +0.43%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 2, 2024

A preview of the GitHub Pages site based on this PR is now available here:

solana-labs.github.io/solana-web3.js-pr-preview/3661/

@steveluscher steveluscher mentioned this pull request Dec 2, 2024
Closed
lorisleiva
lorisleiva approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I didn't know Node would start throwing warning at 10 listeners haha. Thanks for the fix!

packages/event-target-impl/src/index.node.ts
Comment on lines +3 to +8
export const AbortController = class extends globalThis.AbortController {
constructor(...args: ConstructorParameters<typeof globalThis.AbortController>) {
super(...args);
setMaxListeners(Number.MAX_SAFE_INTEGER, this.signal);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason behind setting this inside the AbortController constructor as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signal is an EventTarget, and apparently attaching addEventListener('abort', ...) to it more than 10 times also raises this warning.

@steveluscher Graphite App
Copy link
Collaborator Author

steveluscher commented Dec 4, 2024 •
edited
Loading

Merge activity

  • Dec 4, 9:49 AM PST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 4, 9:50 AM PST: A user merged this pull request with Graphite.

@steveluscher steveluscher merged commit 29d1e28 into master Dec 4, 2024
14 checks passed
@steveluscher steveluscher deleted the 12-02-disable_the_maxlistenersexceededwarning_in_node_when_creating_event_targets_for_internal_use branch December 4, 2024 17:50
@github-actions github-actions bot mentioned this pull request Dec 5, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lorisleiva lorisleiva lorisleiva approved these changes

@mcintyre94 mcintyre94 Awaiting requested review from mcintyre94

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LogSubscription warning MaxListenersExceededWarning: Possible EventTarget memory leak detected.
2 participants
@steveluscher @lorisleiva